feat(web-ui): add template editor to claude config panel#167
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Claude config-template editor and context-aware template application; moves Codex/Claude bridge-pool controls into modal dialogs, adds card-level bridge-pool summaries and trigger buttons, and introduces supporting CSS. ChangesTemplate editor and bridge-pool UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/modules/app.methods.codex-config.mjs`:
- Around line 811-820: The preview gating logic for config applies is using the
shared "preview-config-template-diff" path even when configTemplateContext ===
'claude', causing Claude-mode applies to be blocked/misrepresented; update
applyConfigTemplate() (or the code path that checks
preview-config-template-diff) to skip or bypass the shared diff-preview gate
when this.configTemplateContext === 'claude' (or alternatively propagate the
'claude' context so the preview check uses a Claude-aware gate), ensuring the
Claude branch that calls api('apply-claude-settings-raw', ...) is not forced
through the generic preview gate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e5201af-fa5c-47fd-a115-3c2be8c90e57
📒 Files selected for processing (4)
web-ui/app.jsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.codex-config.mjsweb-ui/partials/index/panel-config-claude.html
📜 Review details
🔇 Additional comments (4)
web-ui/app.js (1)
101-101: LGTM!web-ui/modules/app.methods.claude-config.mjs (1)
269-282: LGTM!web-ui/partials/index/panel-config-claude.html (1)
85-85: LGTM!web-ui/modules/app.methods.codex-config.mjs (1)
561-562: LGTM!
Comment on lines
+811
to
+820
| let res; | ||
| if (this.configTemplateContext === 'claude') { | ||
| res = await api('apply-claude-settings-raw', { | ||
| content: this.configTemplateContent | ||
| }); | ||
| } else { | ||
| res = await api('apply-config-template', { | ||
| template: this.configTemplateContent | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Claude apply path is context-aware, but confirmation preview gating is not.
At Line 812 you route apply correctly, but applyConfigTemplate() still enforces the shared diff-preview gate (preview-config-template-diff) with no Claude context. In Claude mode this can block or mislead apply decisions based on the wrong target.
Suggested frontend-safe fix (bypass codex diff gate for Claude context)
async applyConfigTemplate() {
if (this.configTemplateApplying) {
return;
}
if (!this.configTemplateContent || !this.configTemplateContent.trim()) {
this.showMessage('模板不能为空', 'error');
return;
}
+ const isClaudeTemplate = this.configTemplateContext === 'claude';
// Default to two-step confirmation when the setting is unset.
// (The normalize helper lives in session-actions; keep a safe fallback here.)
- const shouldUseTwoStepConfirm = normalizeConfigTemplateDiffConfirmEnabled(this.configTemplateDiffConfirmEnabled);
+ const shouldUseTwoStepConfirm = !isClaudeTemplate
+ && normalizeConfigTemplateDiffConfirmEnabled(this.configTemplateDiffConfirmEnabled);
const performApply = async () => {
this.configTemplateApplying = true;
try {
let res;
if (this.configTemplateContext === 'claude') {
res = await api('apply-claude-settings-raw', {
content: this.configTemplateContent
});
} else {
res = await api('apply-config-template', {
template: this.configTemplateContent
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/app.methods.codex-config.mjs` around lines 811 - 820, The
preview gating logic for config applies is using the shared
"preview-config-template-diff" path even when configTemplateContext ===
'claude', causing Claude-mode applies to be blocked/misrepresented; update
applyConfigTemplate() (or the code path that checks
preview-config-template-diff) to skip or bypass the shared diff-preview gate
when this.configTemplateContext === 'claude' (or alternatively propagate the
'claude' context so the preview check uses a Claude-aware gate), ensuring the
Claude branch that calls api('apply-claude-settings-raw', ...) is not forced
through the generic preview gate.
- Show pool status summary above local provider title - Add modal dialog for pool management (Codex & Claude) - Add trigger button in local card actions - Remove standalone bridge-pool-panel
- Add applyClaudeLocalBridge() method to enable local bridge - Local card click now calls API to write config to file - Refresh will persist local selection
…ider - Match claude-local when URL contains /bridge/claude-local/ - Prevents creating '导入-127.0.0.1:xxxx' provider on refresh
- Remove local card from Claude config panel - Remove Claude bridge pool modal - Keep Codex local provider functional
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/logic.claude.mjs`:
- Around line 114-117: The local-bridge URL check inside web-ui/logic.claude.mjs
is too strict — the current conditional that inspects normalizedSettings.baseUrl
only matches '/bridge/claude-local/' and misses URLs without a trailing slash;
update the detection logic (the conditional referencing
normalizedSettings.baseUrl in this module) to treat '/bridge/claude-local' and
'/bridge/claude-local/' (and any URL where '/bridge/claude-local' is a path
segment) as matches — e.g., replace the exact include check with a test that
accepts either a trailing slash or end-of-string (or use startsWith or a regex
like '/bridge/claude-local(\/|$)') so the function returns 'claude-local' for
both forms.
In `@web-ui/modules/app.methods.claude-config.mjs`:
- Around line 269-288: The method applyClaudeLocalBridge currently sets
this.currentClaudeConfig = 'claude-local' and writes localStorage before the API
call, causing stale UI state if validation or api('claude-local-bridge-toggle',
{ enable: true }) fails; change the flow in applyClaudeLocalBridge so you first
validate candidates via claudeLocalBridgeCandidateProviders(), then call the
API, and only on successful response set this.currentClaudeConfig =
'claude-local', persist to localStorage, and call
this.refreshClaudeModelContext(); if the API returns an error or throws, do not
modify currentClaudeConfig or localStorage (or revert them if already changed),
and continue to show the existing error messages via this.showMessage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 790b6463-247f-487e-8c8a-77011e759060
📒 Files selected for processing (5)
tests/unit/web-ui-behavior-parity.test.mjsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.html
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/partials/index/panel-config-claude.html
📜 Review details
🔇 Additional comments (3)
web-ui/modules/app.methods.claude-config.mjs (1)
291-304: LGTM!tests/unit/web-ui-behavior-parity.test.mjs (1)
414-429: LGTM!Also applies to: 577-578
web-ui/partials/index/modals-basic.html (1)
175-198: LGTM!
Comment on lines
+114
to
+117
| // 检测本地桥接 URL | ||
| if (typeof normalizedSettings.baseUrl === 'string' && normalizedSettings.baseUrl.includes('/bridge/claude-local/')) { | ||
| return 'claude-local'; | ||
| } |
Contributor
There was a problem hiding this comment.
Local-bridge URL detection is too strict and misses valid forms.
The current check only matches /bridge/claude-local/. URLs without trailing slash (e.g., /bridge/claude-local) won’t match.
Proposed fix
- if (typeof normalizedSettings.baseUrl === 'string' && normalizedSettings.baseUrl.includes('/bridge/claude-local/')) {
+ const normalizedBaseUrl = normalizeClaudeComparableUrl(normalizedSettings.baseUrl).toLowerCase();
+ if (normalizedBaseUrl.includes('/bridge/claude-local')) {
return 'claude-local';
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/logic.claude.mjs` around lines 114 - 117, The local-bridge URL check
inside web-ui/logic.claude.mjs is too strict — the current conditional that
inspects normalizedSettings.baseUrl only matches '/bridge/claude-local/' and
misses URLs without a trailing slash; update the detection logic (the
conditional referencing normalizedSettings.baseUrl in this module) to treat
'/bridge/claude-local' and '/bridge/claude-local/' (and any URL where
'/bridge/claude-local' is a path segment) as matches — e.g., replace the exact
include check with a test that accepts either a trailing slash or end-of-string
(or use startsWith or a regex like '/bridge/claude-local(\/|$)') so the function
returns 'claude-local' for both forms.
Comment on lines
+269
to
+288
| async applyClaudeLocalBridge() { | ||
| this.currentClaudeConfig = 'claude-local'; | ||
| try { localStorage.setItem('currentClaudeConfig', 'claude-local'); } catch (_) {} | ||
| this.refreshClaudeModelContext(); | ||
|
|
||
| const candidates = this.claudeLocalBridgeCandidateProviders(); | ||
| if (candidates.length === 0) { | ||
| return this.showMessage('请先添加并配置至少一个 Claude 提供商', 'error'); | ||
| } | ||
|
|
||
| try { | ||
| const res = await api('claude-local-bridge-toggle', { enable: true }); | ||
| if (res.error) { | ||
| this.showMessage(res.error || '启用本地负载均衡失败', 'error'); | ||
| return; | ||
| } | ||
| this.showMessage('Claude 本地负载均衡已启用', 'success'); | ||
| } catch (e) { | ||
| this.showMessage('启用本地负载均衡失败', 'error'); | ||
| } |
Contributor
There was a problem hiding this comment.
Switching to claude-local before enable succeeds leaves stale UI state.
If validation or claude-local-bridge-toggle fails, the app still persists currentClaudeConfig='claude-local'. Keep current selection unchanged until enable succeeds.
Proposed fix
async applyClaudeLocalBridge() {
- this.currentClaudeConfig = 'claude-local';
- try { localStorage.setItem('currentClaudeConfig', 'claude-local'); } catch (_) {}
- this.refreshClaudeModelContext();
-
const candidates = this.claudeLocalBridgeCandidateProviders();
if (candidates.length === 0) {
return this.showMessage('请先添加并配置至少一个 Claude 提供商', 'error');
}
try {
const res = await api('claude-local-bridge-toggle', { enable: true });
if (res.error) {
this.showMessage(res.error || '启用本地负载均衡失败', 'error');
return;
}
+ this.currentClaudeConfig = 'claude-local';
+ try { localStorage.setItem('currentClaudeConfig', 'claude-local'); } catch (_) {}
+ this.refreshClaudeModelContext();
this.showMessage('Claude 本地负载均衡已启用', 'success');
} catch (e) {
this.showMessage('启用本地负载均衡失败', 'error');
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async applyClaudeLocalBridge() { | |
| this.currentClaudeConfig = 'claude-local'; | |
| try { localStorage.setItem('currentClaudeConfig', 'claude-local'); } catch (_) {} | |
| this.refreshClaudeModelContext(); | |
| const candidates = this.claudeLocalBridgeCandidateProviders(); | |
| if (candidates.length === 0) { | |
| return this.showMessage('请先添加并配置至少一个 Claude 提供商', 'error'); | |
| } | |
| try { | |
| const res = await api('claude-local-bridge-toggle', { enable: true }); | |
| if (res.error) { | |
| this.showMessage(res.error || '启用本地负载均衡失败', 'error'); | |
| return; | |
| } | |
| this.showMessage('Claude 本地负载均衡已启用', 'success'); | |
| } catch (e) { | |
| this.showMessage('启用本地负载均衡失败', 'error'); | |
| } | |
| async applyClaudeLocalBridge() { | |
| const candidates = this.claudeLocalBridgeCandidateProviders(); | |
| if (candidates.length === 0) { | |
| return this.showMessage('请先添加并配置至少一个 Claude 提供商', 'error'); | |
| } | |
| try { | |
| const res = await api('claude-local-bridge-toggle', { enable: true }); | |
| if (res.error) { | |
| this.showMessage(res.error || '启用本地负载均衡失败', 'error'); | |
| return; | |
| } | |
| this.currentClaudeConfig = 'claude-local'; | |
| try { localStorage.setItem('currentClaudeConfig', 'claude-local'); } catch (_) {} | |
| this.refreshClaudeModelContext(); | |
| this.showMessage('Claude 本地负载均衡已启用', 'success'); | |
| } catch (e) { | |
| this.showMessage('启用本地负载均衡失败', 'error'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/app.methods.claude-config.mjs` around lines 269 - 288, The
method applyClaudeLocalBridge currently sets this.currentClaudeConfig =
'claude-local' and writes localStorage before the API call, causing stale UI
state if validation or api('claude-local-bridge-toggle', { enable: true })
fails; change the flow in applyClaudeLocalBridge so you first validate
candidates via claudeLocalBridgeCandidateProviders(), then call the API, and
only on successful response set this.currentClaudeConfig = 'claude-local',
persist to localStorage, and call this.refreshClaudeModelContext(); if the API
returns an error or throws, do not modify currentClaudeConfig or localStorage
(or revert them if already changed), and continue to show the existing error
messages via this.showMessage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
New Features
Style
Behavior